-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: use --syncmode=execution-layer
from op-node
for optimistic pipeline sync
#7552
Conversation
op-reth
now uses --syncmode=execution-layer
from op-node
on starting up
smol meta nit: can we make the PR title imperative 😄 |
op-reth
now uses --syncmode=execution-layer
from op-node
on starting up--syncmode=execution-layer
from op-node
for pipeline sync
crates/rpc/rpc/src/eth/error.rs
Outdated
/// op-node uses case sensitive string comparison to parse this error: | ||
/// https://github.com/ethereum-optimism/optimism/blob/0913776869f6cb2c1218497463d7377cf4de16de/op-service/sources/l2_client.go#L105 | ||
/// | ||
/// Temporary, until a version of https://github.com/ethereum-optimism/optimism/pull/10071 is pushed through that doesn't require this to figure out the EL sync status. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's also track this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} else if error.optimistic_revert_block_number().is_some() { | ||
// engine already set the pipeline unwind target on | ||
// `try_make_sync_target_canonical` | ||
PayloadStatus::from_status(PayloadStatusEnum::Syncing) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unsure here if returning Syncing is the best
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be okay
@@ -668,6 +669,21 @@ where | |||
// threshold | |||
return Some(state.finalized_block_hash) | |||
} | |||
|
|||
// OPTIMISTIC SYNCING |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we feature gate this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or should we do this only for OP (chainspec.is_optimism) for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i mean, its unlikely that it can happen on mainnet, but it can happen on testnets/devnets l1 for sure.
i can add the feature gate or check... but what would be the reason?
1st run of hive tests seemed to fail 2 tests, tried locally and they were all passing so issued a second action and they all passed same as main: https://github.com/paradigmxyz/reth/actions/runs/8930154135 |
eth-holesky: triggered optimistic sync on lighthouse + reth (although low block, instead of near the tip) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
few more doc nits, but overall I think all of this is fine
head_block_hash: head.hash(), | ||
safe_block_hash: head.hash(), | ||
finalized_block_hash: head.hash(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure this was problematic from the start
} else if error.optimistic_revert_block_number().is_some() { | ||
// engine already set the pipeline unwind target on | ||
// `try_make_sync_target_canonical` | ||
PayloadStatus::from_status(PayloadStatusEnum::Syncing) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be okay
@@ -668,6 +669,21 @@ where | |||
// threshold | |||
return Some(state.finalized_block_hash) | |||
} | |||
|
|||
// OPTIMISTIC SYNCING |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or should we do this only for OP (chainspec.is_optimism) for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pending @rkrasiuk
…ipeline sync (paradigmxyz#7552) Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
Adds optimistic sync (unsafe head hash & finalized zero hash). Although a requirement for optimism, it's also an expectation in non OP chains.
PipelineTarget::Unwind(block)
crates/optimism/node/tests/e2e/p2p.rs
has an example scenarioPending: syncing base main from scratch.--
OP specific changes:
https://github.com/paradigmxyz/reth/pull/7552/files#diff-c821f432ea2a3ba2fdad959da77aae18c2b20e4988e032aa856576cfafb3a58a:
op-node
will ignore--syncmode=execution-layer
, if the EL has any finalized block. This makes sure that we don't start with one (as is usual behaviour). Otherwise,op-node
will move on to a CL sync style, feeding us one block at a time from our lowest.The error from
UnknownSafeOrFinalizedBlock
has to start with an upper case since it's what op-node requires: https://github.com/ethereum-optimism/optimism/blob/0913776869f6cb2c1218497463d7377cf4de16de/op-service/sources/l2_client.go#L105. This is temporary until a version of the following PR is pushed through, where the tracking of the EL status is properly tracked.